fix: restore snap and imported srps when changing password #15237

montelaidev · 2025-05-08T14:48:49Z

Description

This PR adds the restoration of first party snap accounts and imported srps when changing password.

Changes:

added restoreImportedSrp restoreSnapAccounts to Vault.js and updated the change password logic to use these.
Added new tests to Vault
Fix deadlock that could happen when awaiting a promise in MultichainSnapClient.

Related issues

Fixes: #15258
Fixes: #15233
Resolves: https://consensyssoftware.atlassian.net/browse/MMMULTISRP-208?atlOrigin=eyJpIjoiMmM0MDMzNDRlYzdlNDJmZTg3ODNhMzUzNzlkOWZmZDIiLCJwIjoiaiJ9

Manual testing steps

Using a fresh wallet.
Create a new solana account
Import a private key
Import a qr hardware wallet
import a ledger wallet
Go to settings --> security
Go through change password
Login to the wallet and see that accounts are still there.

Screenshots/Recordings

Before

After

Pre-merge author checklist

I’ve followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2025-05-08T14:49:02Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

github-actions · 2025-05-09T09:33:55Z

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 7a14c1b
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/402ab50f-d62e-48e4-bed7-6afeb9ee42ed

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

Check the documentation if you have any doubts on how to understand the failure on bitrise

github-actions · 2025-05-09T11:16:46Z

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 9926d8b
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/7a7e9c4f-75b0-414e-992f-5e1e0fcd276f

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

owencraston · 2025-05-14T11:14:03Z

Testing summary

I was unable to test hardware wallets due to QR hardware connections being broken and not having access to a ledger device
In general this feature is working in the sense that the accounts are restored however there are some querks.

Issue 1:

when changing passwords with both Solana accounts and multiple SRPs, the network switched popup shows Solana and then Ethereum while the vault is being recreated. There is nothing really wrong with this but its an unnecessary behaviour and is likely slowing down the app.
This is likely not a result of this PR and more the event driven architecture so we do not need to address this in the PR.

Issue 2:

The accounts are all recreated but the names and indexing is all messed up. As you can see in my before/after screenshot, account 3 (0xbe4)was selected prior to the password change. After the password was changed, the same address is selected (0xbe4) but the name has changed from account 3 to account 2. This is quite confusing, especially when you have many accounts. I tested with 15 accounts before this and the results were even weirder. Yes the data is still present but the user might be confused by the name change.

Issue 3:

Custom account names are not persisted accross password changes.
I tested main and this seems to also be the case on main so this is not a blocker. Just something to call out.

Run 1

Before

After

Run 2

Before

After

owencraston

The code looks good to me. Once we get some clarity on the UX "issues" then we can move forward. Unless we feel that the account name changes are a blocker then more code chages will be required.

Tlees-MMI · 2025-05-15T16:11:13Z

Issue 1) That is fine for now given the issue we're trying to solve. I'll make note of it in a ticket

Tlees-MMI · 2025-05-15T16:12:54Z

Issue 2) We accept the wonky indexing here given the problem we are trying to solve. Spoke about this with @montelaidev and we would need additional logic.

Given we will have more structured hierarchies in ~1 month, I am not as worried about this problem in the immediate term. Especially since it is not a well-worn user flow

Tlees-MMI · 2025-05-15T16:14:31Z

Issue #3) Similar feeling to #2. We merge but I'll write a ticket for it

codecov-commenter · 2025-05-19T10:06:20Z

Codecov Report

Attention: Patch coverage is 86.44068% with 8 lines in your changes missing coverage. Please review.

Project coverage is 69.06%. Comparing base (8351137) to head (9bc477d).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
app/core/Vault.js	85.45%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15237      +/-   ##
==========================================
+ Coverage   68.98%   69.06%   +0.08%     
==========================================
  Files        2408     2409       +1     
  Lines       51681    51728      +47     
  Branches     7779     7777       -2     
==========================================
+ Hits        35652    35727      +75     
+ Misses      13737    13709      -28     
  Partials     2292     2292

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

sonarqubecloud · 2025-05-19T10:07:16Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
80.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

fix: restore snap and imported srps

f343ccf

fix: add fence for multisrp

58deb35

metamaskbot added the team-accounts label May 8, 2025

montelaidev added 7 commits May 8, 2025 23:13

fix: lint

07b8dd0

Merge remote-tracking branch 'origin/main' into fix/mmmultisrp-208

07c40bd

fix: remove logs

7abf52a

feat: add unit tests

c978fbd

fix: lint

b0b005e

fix:lint

1627b9b

fix: tests

7a14c1b

montelaidev added the Run Smoke E2E Requires smoke E2E testing label May 9, 2025

montelaidev marked this pull request as ready for review May 9, 2025 09:33

fix: add await

9926d8b

montelaidev added Run Smoke E2E Requires smoke E2E testing and removed Run Smoke E2E Requires smoke E2E testing labels May 9, 2025

fix: eslint disable

54699b1

metamaskbot added the bitrise-result-ready label May 9, 2025

github-actions bot removed the bitrise-result-ready label May 9, 2025

montelaidev requested a review from a team May 9, 2025 12:54

montelaidev added 2 commits May 9, 2025 22:37

fix: solana snap name after restore

b562e8d

fix: lint and test

ef95336

Andepande added the QA Passed QA testing has been completed and passed label May 9, 2025

Merge branch 'main' into fix/mmmultisrp-208

f2b8c2d

owencraston reviewed May 14, 2025

View reviewed changes

owencraston mentioned this pull request May 14, 2025

[Bug]: Resetting password in settings resets the SRPs and vault #15258

Closed

owencraston previously approved these changes May 15, 2025

View reviewed changes

owencraston added this pull request to the merge queue May 15, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 15, 2025

owencraston added this pull request to the merge queue May 15, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 15, 2025

owencraston added this pull request to the merge queue May 16, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 16, 2025

gantunesr added this pull request to the merge queue May 17, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 17, 2025

gantunesr and others added 3 commits May 17, 2025 23:16

Merge branch 'main' into fix/mmmultisrp-208

eb9a54b

Merge remote-tracking branch 'origin/main' into fix/mmmultisrp-208

66d9386

fix: test

9bc477d

montelaidev dismissed owencraston’s stale review via 9bc477d May 19, 2025 09:53

owencraston approved these changes May 19, 2025

View reviewed changes

montelaidev added this pull request to the merge queue May 19, 2025

Merged via the queue into main with commit 8897c2c May 19, 2025
41 checks passed

montelaidev deleted the fix/mmmultisrp-208 branch May 19, 2025 13:58

github-actions bot locked and limited conversation to collaborators May 19, 2025

metamaskbot added the release-7.48.0 Issue or pull request that will be included in release 7.48.0 label May 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: restore snap and imported srps when changing password #15237

fix: restore snap and imported srps when changing password #15237

montelaidev commented May 8, 2025 •

edited by owencraston

Loading

github-actions bot commented May 8, 2025

github-actions bot commented May 9, 2025 •

edited by metamaskbot

Loading

github-actions bot commented May 9, 2025 •

edited by metamaskbot

Loading

owencraston commented May 14, 2025

owencraston left a comment

Tlees-MMI commented May 15, 2025

Tlees-MMI commented May 15, 2025

Tlees-MMI commented May 15, 2025 •

edited

Loading

codecov-commenter commented May 19, 2025

sonarqubecloud bot commented May 19, 2025

fix: restore snap and imported srps when changing password #15237

fix: restore snap and imported srps when changing password #15237

Conversation

montelaidev commented May 8, 2025 • edited by owencraston Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented May 8, 2025

github-actions bot commented May 9, 2025 • edited by metamaskbot Loading

Bitrise

github-actions bot commented May 9, 2025 • edited by metamaskbot Loading

Bitrise

owencraston commented May 14, 2025

Testing summary

Issue 1:

Issue 2:

Issue 3:

Run 1

Before

After

Run 2

Before

After

owencraston left a comment

Choose a reason for hiding this comment

Tlees-MMI commented May 15, 2025

Tlees-MMI commented May 15, 2025

Tlees-MMI commented May 15, 2025 • edited Loading

codecov-commenter commented May 19, 2025

Codecov Report

sonarqubecloud bot commented May 19, 2025

Quality Gate passed

montelaidev commented May 8, 2025 •

edited by owencraston

Loading

github-actions bot commented May 9, 2025 •

edited by metamaskbot

Loading

github-actions bot commented May 9, 2025 •

edited by metamaskbot

Loading

Tlees-MMI commented May 15, 2025 •

edited

Loading